Enable additional ccxt params and http headers #140
Conversation
There was a problem hiding this comment.
@Reidmcc this is very useful, will try and merge it into master tomorrow. some comments inline.
api/exchange.go
Outdated
| @@ -14,6 +14,18 @@ type ExchangeAPIKey struct { | |||
| Secret string | |||
| } | |||
|
|
|||
| // CcxtParam specifies an additional ccxt parameter | |||
| type CcxtParam struct { | |||
There was a problem hiding this comment.
@Reidmcc mostly looks good. can you rename CcxtParam to ExchangeParam everywhere?
The exchange interface abstracts away the idea of ccxt / sdex / kraken from the calling code.
api/exchange.go
Outdated
| Value string | ||
| } | ||
|
|
||
| // ExchangeHeader specifies additional HTTP headers for centralized exchanges |
There was a problem hiding this comment.
same logic as comment above, this comment should not say "centralized exchanges" since it abstracts away the concept of the underlying exchange.
cmd/trade.go
Outdated
| @@ -187,8 +187,24 @@ func makeExchangeShimSdex( | |||
| }) | |||
| } | |||
|
|
|||
| ccxtParams := []api.CcxtParam{} | |||
There was a problem hiding this comment.
similarly, this here should be exchangeParams instead of ccxtParams
plugins/ccxtExchange_test.go
Outdated
| @@ -14,6 +14,8 @@ import ( | |||
|
|
|||
| var supportedExchanges = []string{"binance"} | |||
| var emptyAPIKey = api.ExchangeAPIKey{} | |||
| var emptyParams = api.CcxtParam{} | |||
| var emptyHeader = api.ExchangeHeader{} | |||
There was a problem hiding this comment.
@Reidmcc there are a few tests here (ccxtExchange_test.go) that include the list of "tested" exchanges (example: TestGetOrderConstraints_Ccxt_Precision). Can you add coinbasepro to that test?
|
@nikhilsaraf changes made |
|
ok, apparently I need to go over the |
plugins/ccxtExchange_test.go
Outdated
| exchangeName: "coinbasepro", | ||
| pair: &model.TradingPair{Base: model.XLM, Quote: model.USD}, | ||
| wantPricePrecision: 6, | ||
| wantVolPrecision: 0, |
There was a problem hiding this comment.
can you share the link where to find precision values for coinbasepro here? may be useful to put this as a comment inside each of these test cases
(don't worry about the kraken/binance cases, I'll add these links in)
…into enable-ccxt-params
|
@nikhilsaraf alright, it's fixed. It was failing because of the coinbase pro precision problems |
…ar-deprecated#141, closes stellar-deprecated#152 (stellar-deprecated#153) renamed `MIN_CENTRALIZED_BASE_VOLUME` to `CENTRALIZED_MIN_BASE_VOLUME_OVERRIDE` in trader config, see sample config files for latest examples
|
@Reidmcc I've made some changes to merge the master branch in and fix some remaining comments and convention. I think this is pretty much good to go, could you test it out once? -- the precision override logic should also be included here so it should all just work. Also, I could not find the coinbase pro API docs that show the headers and params that are needed. Do you have the link handy so I can add it in? |
There was a problem hiding this comment.
@Reidmcc Merging this in to master -- please test it and let me know if it's working for you as expected and we can mark the coinbasepro integration as tested, thanks!
|
@nikhilsaraf the parameters pass in fine, but the volume override isn't working correctly. Kelp still calculates volumes to SDEX precision, and when inverting for buy orders sometimes ends up with low over-precise values that can fail min volume tests if you set your volume to the minimum, i.e. if you set your volume to 1.0, Kelp may invert the volume to 0.99999981, and then reject the order at |
|
@nikhilsaraf cbpro header reqs are here https://docs.pro.coinbase.com/#creating-a-request |
This pr implements the feature requested in #139 by allowing users to specify any additional ccxt parameters and/or http headers they may need to add to their API requests. I have live tested it on Coinbase Pro, so if this is implemented we can marked Coinbase Pro as tested.